-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change pdata generated types to use type definition instead of aliases #5936
Conversation
00dbeab
to
c065c0c
Compare
Codecov Report
@@ Coverage Diff @@
## main #5936 +/- ##
==========================================
+ Coverage 91.90% 92.30% +0.39%
==========================================
Files 200 212 +12
Lines 12414 13249 +835
==========================================
+ Hits 11409 12229 +820
- Misses 793 803 +10
- Partials 212 217 +5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
c065c0c
to
decef68
Compare
decef68
to
931aaf7
Compare
56b0107
to
beea276
Compare
Change the way how objects are generated in `pdata`. Previously the pdata objects were generated in the `internal` package and aliases were created in the public packageas. This PR changes this, by creating only "wrappers" object inside the internal package, and the public type is a type def of the internal type: ```golang package internal type LogRecord struct { orig *otlplogs.LogRecord } func GetOrigLogRecord(ms LogRecord) *otlplogs.LogRecord { return ms.orig } func NewLogRecord(orig *otlplogs.LogRecord) LogRecord { return LogRecord{orig: orig} } ``` ```golang package plog type LogRecord internal.LogRecord ``` With this approach, we still do not allow users access to the internal origin, which allows us flexibility to change to other representation (something like lazy proto, etc), but improves documentation of the pdata packages, see [current documentation](https://pkg.go.dev/go.opentelemetry.io/collector/[email protected]/plog). Signed-off-by: Bogdan <[email protected]>
beea276
to
474d968
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just couple of nits. Otherwise LGTM.
This is a great solution! I believe the only downside is that now we have some computational overhead spent of type conversions, right?
I believe the compiler should be smart enough to not have to do this :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The mistake happened in open-telemetry#5936 Signed-off-by: Bogdan <[email protected]>
The mistake happened in #5936 Signed-off-by: Bogdan <[email protected]> Signed-off-by: Bogdan <[email protected]>
Change the way how objects are generated in
pdata
. Previously the pdata objects were generated in theinternal
package and aliases were created in the public packageas.This PR changes this, by creating only "wrappers" object inside the internal package, and the public type is a type def of the internal type:
With this approach, we still do not allow users access to the internal origin, which allows us flexibility to change to other representation (something like lazy proto, etc), but improves documentation of the pdata packages, see current documentation.
Signed-off-by: Bogdan [email protected]